Skip to content

chore: implement phase 2 - #2780

Merged
Tofel merged 4 commits into
dx-5122-alerts-assertion-p1from
dx-5122-alerts-assertion-p2
Sep 9, 2026
Merged

chore: implement phase 2#2780
Tofel merged 4 commits into
dx-5122-alerts-assertion-p1from
dx-5122-alerts-assertion-p2

Conversation

@Tofel

@Tofel Tofel commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Fixes retry-error conflation, measures full poll latency, and hardens the Source test doubles for concurrent use.

Review focus: source.go (error vs retry classification, latency timing) and source_fake_test.go (concurrency safety).

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

⚠️ API Diff Results - github.com/smartcontractkit/chainlink-testing-framework/grafana-alertcheck

⚠️ Breaking Changes (1)

package github (1)
  • com/smartcontractkit/chainlink-testing-framework/grafana-alertcheck/cmd/grafana-alertcheck — 🗑️ Removed

✅ Compatible Changes (1)

package github (1)
  • com/smartcontractkit/chainlink-testing-framework/grafana-alertcheck/cmd — ➕ Added

📄 View full apidiff report

@Tofel
Tofel force-pushed the dx-5122-alerts-assertion-p1 branch from b406d5a to d4124cb Compare September 1, 2026 11:25
@Tofel
Tofel force-pushed the dx-5122-alerts-assertion-p2 branch from bbd2ebe to 799df17 Compare September 1, 2026 11:25
// skewHardLimit is one of §5's filled-in values (basis: §16; §22.11 asserts
// 120s errors, 30s does not). It belongs in schedule.go's named-constants
// block once P4 exists; defined here because P2 needs it first.
const skewHardLimit = 60 * time.Second

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clock skew is only ever needed to place the runner-defined window edges [from, to+grace] onto Grafana's own timeline, so it's a single-point reconciliation, not a factor in any duration-based check. The 60s hard limit is therefore a sanity gate, not a correction: skew is estimated as serverDate − (t_send+t_recv)/2 with error RTT/2, and that estimation is only trusted while the two clocks are in rough agreement. Past 60s the offset is treated as a broken or misconfigured clock, so the gate fails closed rather than align fine-grained window work against a clock it no longer trusts.

// verified against (§2.7 control 2, §21.5): >= 13.0.0, < 14.0.0.
var (
supportedGrafanaMin = grafanaVersion{13, 0, 0}
supportedGrafanaMax = grafanaVersion{14, 0, 0} // exclusive

@Tofel Tofel Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better safe than sorry, we don't want to risk subtle behaviour changes.

@Tofel
Tofel force-pushed the dx-5122-alerts-assertion-p1 branch from d4124cb to 96000d4 Compare September 2, 2026 09:45
@Tofel
Tofel force-pushed the dx-5122-alerts-assertion-p2 branch from 4159c2f to 3894d9b Compare September 2, 2026 09:45
@Tofel
Tofel force-pushed the dx-5122-alerts-assertion-p1 branch from 96000d4 to 730e632 Compare September 4, 2026 15:03
@Tofel
Tofel force-pushed the dx-5122-alerts-assertion-p2 branch from 3894d9b to 96ebd78 Compare September 4, 2026 15:03
@Tofel
Tofel force-pushed the dx-5122-alerts-assertion-p1 branch from 730e632 to ca6eed4 Compare September 7, 2026 09:35
@Tofel
Tofel force-pushed the dx-5122-alerts-assertion-p2 branch from 96ebd78 to be0dd37 Compare September 7, 2026 09:35
@Tofel
Tofel marked this pull request as ready for review September 7, 2026 09:36
@Tofel
Tofel requested a review from a team as a code owner September 7, 2026 09:36
Copilot AI lite review requested due to automatic review settings September 7, 2026 09:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

It introduces unbounded response-body reads in the HTTP source and the “goroutine-safe” fake returns internal slices without defensive copies, both of which can lead to reliability/concurrency issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR introduces the Phase 2 “Source” layer for the Grafana alert gate, including stricter transport vs hard-error classification, full end-to-end poll latency measurement, and concurrency-hardened test doubles.

Changes:

  • Add a production httpSource implementing Source, including Grafana version parsing/range enforcement, Date/skew validation, and retry-with-backoff for retryable transport failures.
  • Add comprehensive unit tests for skew/date-header hard failures, retry exhaustion semantics, and timing arithmetic (skew bound + full-body latency).
  • Expand Prometheus duration parsing negative test coverage with additional invalid inputs.
File summaries
File Description
grafana-alertcheck/internal/gate/source.go Adds Source + httpSource, request timing/skew logic, and transport retry/backoff behavior.
grafana-alertcheck/internal/gate/source_test.go Adds end-to-end tests for version/state/definitions, retry behavior, and timing/skew calculations.
grafana-alertcheck/internal/gate/source_fake_test.go Adds goroutine-safe fake clock(s) and scripted fake Source for higher-phase tests.
grafana-alertcheck/internal/gate/duration_test.go Adds more invalid-duration inputs to strengthen parser error coverage.
Review details

Suppressed comments (1)

grafana-alertcheck/internal/gate/source_fake_test.go:132

  • RuleState returns next.obs (including obs.Rules) directly. Since the last scripted entry repeats, any caller mutation of the returned slice can leak into future calls and also create cross-goroutine races; copy the Rules slice before returning.
	next := q[0]
	if len(q) > 1 {
		f.states[title] = q[1:]
	}
	return next.obs, next.err
}
  • Files reviewed: 4/4 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread grafana-alertcheck/internal/gate/source.go Outdated
Comment thread grafana-alertcheck/internal/gate/source_fake_test.go
Comment thread grafana-alertcheck/internal/gate/source.go Outdated
@Tofel
Tofel force-pushed the dx-5122-alerts-assertion-p1 branch from ca6eed4 to afcf257 Compare September 7, 2026 09:46
@Tofel
Tofel force-pushed the dx-5122-alerts-assertion-p2 branch from be0dd37 to 6f29f79 Compare September 7, 2026 09:46
Fix retry-error conflation, measure full poll latency, and harden Source test doubles for concurrency.
@Tofel
Tofel force-pushed the dx-5122-alerts-assertion-p2 branch from 6f29f79 to ba93502 Compare September 7, 2026 15:02
@Tofel
Tofel removed this pull request from stack #2791 September 9, 2026 09:52
@Tofel Tofel closed this Sep 9, 2026
* chore: implement phase 3

Add Resolve() for alert name resolution (uid:/Title/Folder/Title/
Folder/Group/Title forms, UID collapse, no-match suggestions) and the
grafana-alertcheck CLI's list subcommand, the first runnable piece of
the gate.

Incorporates review fixes: reject empty path segments in classifyForm,
guard uid: against an empty suffix, scope the no-match rule count and
suggestions to supported rule kinds only, and exit 0 on -h/--help.

* chore: enhance unit tests

* chore: implement phase 4 (#2782)

* chore: implement phase 4

Add per-rule poll timings, scheduler, and budget check (P4).

- schedule.go: DeriveTimings, Scheduler, CheckBudget (§5)
- Address review: add Folder/Title resolve test, rename
  CheckBudget's minPollEvery to tightestUID

* chore: enhance unit tests

* chore: address code review comments

* chore: implement phase 5 (#2783)

* chore: implement phase 5

Add the JSONL evidence log (P5).

- log.go: Header/Poll records, reduction, H2 transition markers,
  §3.2 verification, append-only Writer with flock, ReadLog
- flock_unix.go: non-blocking exclusive lock, unix only
- schedule.go: DeriveTimingsFromLog — log-mode cadence comes from the
  header, never from the definitions

* chore: remove unix build tags

* chore: address code review comments

* chore: implement phase 6 (#2784)

* chore: implement phase 6

Invariant defended: H2. The one question: can watch return success over a
window that nothing is recording?

Watch() records the first observation of each non-skipped rule, then detaches
a child that polls at the cadence in the header. The parent returns only after
the child reports ready on an inherited pipe, and writes the pidfile after
that. A clean stop writes the sentinel; a hard error does not.

* chore: add a unit test, remove build tags

* chore: address code review comments

* chore: implement phase 7 (#2785)

* chore: implement phase 7

Invariant defended: H3. The one question: can a rule be called alive
because it looked alive one poll ago?

proveCoverage (grafana-alertcheck/internal/gate/coverage.go) is the pure
coverage function: nine checks over one rule's polls — sentinel,
from-bounds, heartbeat continuity, health error/nodata, liveness,
in-window pause, rule absence, KeepLast. Liveness is absolute, never a
delta. Cross-domain comparisons translate by each poll's own skew and
widen boundary segments by its skew bound, fail-closed.

* chore: enhance unit tests

* chore: address code review comments

* chore: implement phase 8 (#2786)

* chore: implement phase 8

Invariant defended: H6/H7. The one question: can a violation ever
outrank an unobservable rule, or can a pass happen without
Violations empty and err nil?

Adds classify.go: the pure per-instance classifier (outcome table,
preexisting policy, BadFor) and decide(), the seam combining
proveCoverage with those timelines under one Policy. Consolidates
rule-poll filtering and skew translation onto pollsForRule/runnerTime,
shared with coverage.go.

* chore: rename some vars + add unit tests

* chore: address code review comments

* chore: implement phase 9 (#2787)

* chore: implement phase 9

Invariant defended: H5/H7. The one question: can check report a pass
over a window it did not prove?

Check() is the I/O shell around the pure decide(). Single-step
synthesizes the header and its own sentinel, so no mode flag reaches
the pure layer. Log mode stops the recorder before the one full read.

The header, not a definition re-resolved after the window closed, is
the authority for what was paused when the window opened — it decides
`skipped`, the drain set, and the transitionGrace max. The flock, not
the pidfile, is the authority for whether a writer still exists.

* chore: remove unix build tag

* chore: implement phase 10 (#2788)

* Wire watch/check subcommands to the gate library, with a table+JSON
renderer and H6/H7 exit-code mapping. Extend Result with per-rule/global
thresholds and a real skew bound; export SkewHardLimit; reject --states
normal.

* chore: fix goreleaser.yaml and add version command

* chore: implement phase 11 (#2789)

* chore: implement phase 11

Add coverage.go's declared-KeepLast check (no_data_state/exec_err_state,
not just an observed reason) and close the remaining §22 gaps: newly_bad's
no-early-exit clock assertion, a recorder-mode gap right after the deploy,
a rule's own coverage gap overriding its own recovery, a genuinely
skew-discriminating staleness test, exit-2 consequences on two
Reason-only coverage tests, and end-to-end checks for log-name
collapse, a truncated log, and the real watch-written state histogram.

* chore: address code review comments

* chore: more concise comments (#2790)

* chore: more concise comments

* fix: merge conflict

* chore: shorten comments

* fix: resolve conflict

* chore: use testify's require in tests (#2792)

* chore: use testify's require in tests

* chore: move remaining assumptions to testify

* chore: address code review comments

* chore: fix logging and std out printing (#2798)

* chore: fix logging and std out printing

* chore: truncate to seconds when comparing from time

* chore: add centralized docs (#2802)

* chore: add centralized docs

* chore: further update docs

* chore: address code review comments (#2807)

* chore: address code review comments

* chore: get rid of goreleaser
@Tofel Tofel reopened this Sep 9, 2026
@Tofel
Tofel merged commit 9b65a7f into dx-5122-alerts-assertion-p1 Sep 9, 2026
59 of 60 checks passed
@Tofel
Tofel deleted the dx-5122-alerts-assertion-p2 branch September 9, 2026 10:01
Tofel added a commit that referenced this pull request Sep 9, 2026
* chore: implement phase 1

Strict parsers for the state and ruler endpoints (H1), a Prometheus-style
duration parser, and fixtures sliced from real Grafana 13.1.0 payloads
covering every required/optional-field and must-error case, including the
"Normal (NoData)"/"Normal (Error)" composite reason states found live in the
current fleet capture (not in the original plan's vocabulary).

* chore: apply code review comments

* chore: implement phase 2 (#2780)

* chore: implement phase 2

Fix retry-error conflation, measure full poll latency, and harden Source test doubles for concurrency.

* chore: enhance unit tests

* chore: address code review comments

* chore: implement phase 3 (#2781)

* chore: implement phase 3

Add Resolve() for alert name resolution (uid:/Title/Folder/Title/
Folder/Group/Title forms, UID collapse, no-match suggestions) and the
grafana-alertcheck CLI's list subcommand, the first runnable piece of
the gate.

Incorporates review fixes: reject empty path segments in classifyForm,
guard uid: against an empty suffix, scope the no-match rule count and
suggestions to supported rule kinds only, and exit 0 on -h/--help.

* chore: enhance unit tests

* chore: implement phase 4 (#2782)

* chore: implement phase 4

Add per-rule poll timings, scheduler, and budget check (P4).

- schedule.go: DeriveTimings, Scheduler, CheckBudget (§5)
- Address review: add Folder/Title resolve test, rename
  CheckBudget's minPollEvery to tightestUID

* chore: enhance unit tests

* chore: address code review comments

* chore: implement phase 5 (#2783)

* chore: implement phase 5

Add the JSONL evidence log (P5).

- log.go: Header/Poll records, reduction, H2 transition markers,
  §3.2 verification, append-only Writer with flock, ReadLog
- flock_unix.go: non-blocking exclusive lock, unix only
- schedule.go: DeriveTimingsFromLog — log-mode cadence comes from the
  header, never from the definitions

* chore: remove unix build tags

* chore: address code review comments

* chore: implement phase 6 (#2784)

* chore: implement phase 6

Invariant defended: H2. The one question: can watch return success over a
window that nothing is recording?

Watch() records the first observation of each non-skipped rule, then detaches
a child that polls at the cadence in the header. The parent returns only after
the child reports ready on an inherited pipe, and writes the pidfile after
that. A clean stop writes the sentinel; a hard error does not.

* chore: add a unit test, remove build tags

* chore: address code review comments

* chore: implement phase 7 (#2785)

* chore: implement phase 7

Invariant defended: H3. The one question: can a rule be called alive
because it looked alive one poll ago?

proveCoverage (grafana-alertcheck/internal/gate/coverage.go) is the pure
coverage function: nine checks over one rule's polls — sentinel,
from-bounds, heartbeat continuity, health error/nodata, liveness,
in-window pause, rule absence, KeepLast. Liveness is absolute, never a
delta. Cross-domain comparisons translate by each poll's own skew and
widen boundary segments by its skew bound, fail-closed.

* chore: enhance unit tests

* chore: address code review comments

* chore: implement phase 8 (#2786)

* chore: implement phase 8

Invariant defended: H6/H7. The one question: can a violation ever
outrank an unobservable rule, or can a pass happen without
Violations empty and err nil?

Adds classify.go: the pure per-instance classifier (outcome table,
preexisting policy, BadFor) and decide(), the seam combining
proveCoverage with those timelines under one Policy. Consolidates
rule-poll filtering and skew translation onto pollsForRule/runnerTime,
shared with coverage.go.

* chore: rename some vars + add unit tests

* chore: address code review comments

* chore: implement phase 9 (#2787)

* chore: implement phase 9

Invariant defended: H5/H7. The one question: can check report a pass
over a window it did not prove?

Check() is the I/O shell around the pure decide(). Single-step
synthesizes the header and its own sentinel, so no mode flag reaches
the pure layer. Log mode stops the recorder before the one full read.

The header, not a definition re-resolved after the window closed, is
the authority for what was paused when the window opened — it decides
`skipped`, the drain set, and the transitionGrace max. The flock, not
the pidfile, is the authority for whether a writer still exists.

* chore: remove unix build tag

* chore: implement phase 10 (#2788)

* Wire watch/check subcommands to the gate library, with a table+JSON
renderer and H6/H7 exit-code mapping. Extend Result with per-rule/global
thresholds and a real skew bound; export SkewHardLimit; reject --states
normal.

* chore: fix goreleaser.yaml and add version command

* chore: implement phase 11 (#2789)

* chore: implement phase 11

Add coverage.go's declared-KeepLast check (no_data_state/exec_err_state,
not just an observed reason) and close the remaining §22 gaps: newly_bad's
no-early-exit clock assertion, a recorder-mode gap right after the deploy,
a rule's own coverage gap overriding its own recovery, a genuinely
skew-discriminating staleness test, exit-2 consequences on two
Reason-only coverage tests, and end-to-end checks for log-name
collapse, a truncated log, and the real watch-written state histogram.

* chore: address code review comments

* chore: more concise comments (#2790)

* chore: more concise comments

* fix: merge conflict

* chore: shorten comments

* fix: resolve conflict

* chore: use testify's require in tests (#2792)

* chore: use testify's require in tests

* chore: move remaining assumptions to testify

* chore: address code review comments

* chore: fix logging and std out printing (#2798)

* chore: fix logging and std out printing

* chore: truncate to seconds when comparing from time

* chore: add centralized docs (#2802)

* chore: add centralized docs

* chore: further update docs

* chore: address code review comments (#2807)

* chore: address code review comments

* chore: get rid of goreleaser
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants